Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Experiment: RichText: Format content from tree value #7583

Closed
wants to merge 2 commits into from

Conversation

aduth
Copy link
Member

@aduth aduth commented Jun 27, 2018

Related: #3713

Note: This pull request is considered to be a failed experiment and will be closed immediately upon open, serving only as reference for future effort / discussion.

This pull request seeks to explore using TinyMCE's tree format as the base source from which to generate content, leveraging its own empty content filtering rather than trying to recreate it in Gutenberg. It's tangentially related to #7482 in trying to be more aware of block splitting to reuse the existing value on an empty new block. Further, it tries to consolidate splitting behaviors into relying on only TinyMCE's NewBlock event, treating paragraphs as top-level nodes from which new blocks are to be generated (see also #3713 (comment)).

Unfortunately, this seems to have a noticeable performance degradation on simple typing within a paragraph. Exploring further, it appears that getContent( { format: 'tree' } ) incurs a fresh parse on the innerHTML of the editor node to generate the node tree. This is in contrast to our existing implementation which simply walks the DOM and tries to filter empty nodes, which is trivial in comparison.

Useful notes for future consideration:

  • Unlike Editable: separate out content ops and switch to using tinymce tree output #3713, I didn't try to pull in dom-react to do back-mapping of attributes, because I was left wondering: Should we expect any attributes to be assigned to elements in rich-text? If not, we could consider simplifying our domToElement implementation, avoiding nodeListToReact and simply creating elements directly via createElement on the nodeName.toLowerCase() and its childNodes (perhaps helpful for RichText: Use a simplified format for rich text values #7476).
  • The included changes to unset paddEmpty from the editor elements schema can prevent new blocks from including being empty yet assigned as technically a value of [ ' ' ]
  • I'm still not convinced we're ever hitting the NewBlock event in current master (commented as such at Editable: Enter splits the current paragraph into two blocks #409 (comment))
  • We can optimize concatChildren to consider two sets of children which are both strings to be a single set of one concatenated string child (helps when comparing the RichText value as having been changed if value changes from e.g. [ 'Foo' ] to [ 'Foo', '' ] (merging two paragraphs into one when the second paragraph has no text of its own).

@aduth aduth added [Component] TinyMCE [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Type] Performance Related to performance efforts labels Jun 27, 2018
@aduth aduth requested review from youknowriad and ellatrix June 27, 2018 18:35
@aduth aduth closed this Jun 27, 2018
@aduth aduth deleted the try/tinymce-content-from-tree-format branch June 27, 2018 18:35
@aduth
Copy link
Member Author

aduth commented Jun 28, 2018

Unlike #3713, I didn't try to pull in dom-react to do back-mapping of attributes, because I was left wondering: Should we expect any attributes to be assigned to elements in rich-text?

Yes, we should: Namely, the <a href="..."></a>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Type] Performance Related to performance efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant